Repository navigation
Public snapshot codec and snapshot::write_file for a standalone store (#1907) - #1922
Conversation
…#1907) - snapshot::codec: property_to_json / json_to_property move out of snapshot/mod.rs and become pub, so a converter writing or reading .sgsnap (samyama convert, #1905) calls them instead of copying the __type encoding. The bodies are unchanged. - snapshot::write_file(store, path, &WriteOptions) -> ExportStats: creates parent directories, writes <path>.tmp, syncs it and renames it over path, so a failed or interrupted export never leaves a truncated .sgsnap; removes the temporary file on error; returns the export's own stats, dropped included. - ADR-022 gains "Typed values" (every __type tag, its fields and the reading rules) and "Writing a file from a standalone store". Tests: codec round-trips one value of every PropertyValue variant, through serde_json::Value and through text, with an exhaustive match so a new variant fails to compile until it is covered. tests/snapshot_write_file.rs writes a graph with 3 labels, 2 edge types, typed node properties and an edge property, imports it into a fresh store and compares node by node and edge by edge; checks an existing file is replaced whole and that a write that cannot happen errors and leaves nothing behind.
Kalyanpulikanti-01
left a comment
Kalyanpulikanti-01
left a comment
There was a problem hiding this comment.
Approving. Two details here are better than they needed to be.
The exhaustiveness guard actually works. one_of_each() ends in a match over the values with no wildcard and all 15 arms, so adding a variant to PropertyValue fails to compile until it has a test value — and therefore a round trip through the codec. That is the right mechanism for this specific risk: a codec whose test list is maintained by hand drifts silently the moment someone adds a type, and the symptom is a property that survives in memory and vanishes through a snapshot. Paired with assert_eq!(values.len(), 15) it also catches the lazier failure of deleting one entry and duplicating another.
the_round_trip_survives_serialization_to_text is the test most such codecs omit, and the comment says exactly why it matters: a value that round-trips as a serde_json::Value but not through text is still lost, because text is what a .sgsnap actually contains. Float formatting and large integers are the usual casualties.
The atomic write is done properly. Temp file → flush → fsync → rename, plus a directory sync on Unix so the rename itself is durable, and the temp file removed on failure with any previous path intact. The directory sync is the step normally skipped, and skipping it means the rename can be lost on power failure while the data file is intact — a half-committed export that looks complete. The test asserting no .tmp is left beside the file, and that a write whose parent path is a file errors leaving nothing behind, covers both directions.
Making the codec public rather than letting samyama convert copy it is the whole point of the issue, and moving property_to_json/json_to_property unchanged so mod.rs re-imports them keeps the blast radius at zero. The ADR-022 "Typed values" section is what makes a second implementation possible: listing the 8 tags with units is table stakes, but the reading rules — missing required field reads as a plain map except Duration, unknown tag reads as a plain map, strings trimmed — are the part a reimplementor would otherwise have to infer from behaviour. Naming the inherent ambiguity (a user map shaped like a typed value reads back as that value) rather than pretending the format is unambiguous is the honest thing to document; it's a real wart and now a known one.
Comparing by uid rather than id in tests/snapshot_write_file.rs because import remaps ids is correct and easy to get wrong.
No requests. One idle observation: write_file returns ExportStats with dropped included, which means a caller can discard a lossy export silently — the same shape I raised on #1921's RdfImportReport. If that type picks up #[must_use], ExportStats probably wants it for the same reason, and that's a single follow-up across the three report types rather than anything for this PR.
Fixes #1907. Part of #1905.
samyama convert(#1905) builds a graph in process and writes it as.sgsnapwithout a server. This adds the two pieces the issue lists as missing.1.
snapshot::codecis publicproperty_to_jsonandjson_to_propertydefine the{"__type": "Date", "days": …}encoding for every value JSON has no type for. They were private insrc/snapshot/mod.rs, so any other reader or writer had to copy them, and a copy drifts.src/snapshot/codec.rsand arepub.mod.rsimports them, so every existing caller and test is untouched.docs/ADR/ADR-022-snapshot-format.mdgains a "Typed values" section. It lists all 8 tags (DateTime,Date,LocalTime,Time,LocalDateTime,ZonedDateTime,Duration,Vector) with their fields, units and meaning. It also gives the reading rules a second implementation needs to agree with this one:Duration;2.
snapshot::write_file<path>.tmp, flushes andfsyncs it, then renames it overpath. On Unix it also syncs the directory so the rename is durable. A failed export removes the temporary file and leaves any previouspathintact.ExportStats,droppedincluded.WriteOptions { compression_level }defaults toDEFAULT_SNAPSHOT_COMPRESSION.Tests (the issue's acceptance criteria)
src/snapshot/codec.rs): one value of each of the 15PropertyValuevariants round-trips throughserde_json::Valueand through serialized text. The value list sits beside an exhaustivematchwith no wildcard, so adding a variant toPropertyValuewon't compile until it has a test value.write_file→import_tenant(tests/snapshot_write_file.rs):uidbecause import remaps ids;.tmpis left beside the file;__typetags.cargo check --workspace --tests --examples --benchesis clean, and the lint ratchet is under its ceiling.Not in scope
As the issue says, there is no streaming writer (records written while the input is still being read, with no
GraphStore). That gets a separate issue once an input doesn't fit in memory.